Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spicy_add_analyzer: Discourage SCRIPTS usage #114

Closed
wants to merge 1 commit into from
Closed

spicy_add_analyzer: Discourage SCRIPTS usage #114

wants to merge 1 commit into from

Conversation

awelzel
Copy link
Contributor

@awelzel awelzel commented Jun 19, 2024

The SCRIPTS parameter hasn't been functional for a long time and it hasn't been obvious where exactly to install these scripts. They aren't directly comparable to classic plugin's scripts as a Spicy analyzer doesn't have its own directory.

For now, punt on a decision and reference zkg through a warning to keep expectations low and avoid surprises that SCRIPTS isn't working.

@awelzel
spicy_add_analyzer: Discourage SCRIPTS usage
c3b020c 
The SCRIPTS parameter hasn't been functional for a long time and it hasn't
been obvious where exactly to install these scripts.
They aren't directly comparable to classic plugin's scripts as a Spicy
analyzer doesn't have its own directory.

For now, punt on a decision and reference zkg through a warning to keep
expectations low and avoid surprises that SCRIPTS isn't working.
@awelzel awelzel requested a review from rsmmr June 19, 2024 13:24
@awelzel
Copy link
Contributor Author

awelzel commented Jun 19, 2024

@rsmmr - this feels a bit lazy and @bbannier suggested the explicit SCRIPTS could actually be nice, but seems the non-working situation and the fact that zkg should/can manage this might be "good for now".

@rsmmr
Copy link
Member

rsmmr commented Jun 19, 2024 •
edited
Loading

I do think we need some kind of solution here that allows people to install scripts via cmake. Practically all analyzers come with scripts, and just not installing them isn't very useful. While zkg is one way out, we do still allow "make install" for the analyzer itself; the CMake has explicit install() commands for that. Not offering that for the scripts doesn't feel very helpful to me. If it's via SCRIPTS or some install_scripts() function doesn't really matter I think. The catch of course is finding the right destination for the installed scripts, not really sure about that myself.

@awelzel
Copy link
Contributor Author

awelzel commented Jun 19, 2024

If it's via SCRIPTS or some install_scripts() function doesn't really matter I think. The catch of course is finding the right destination for the installed scripts, not really sure about that myself.

As mentioned in the ticket - with install_scripts() my thought was that share/zeek or share/zeek/site/ could be the base, but the concrete path left to the plugin/analyzer developer. But that's rather unhelpful and likely to result in inconsistencies it's not even worth providing - SCRIPTS could be more opinionated.

@ckreibich - any thoughts here? We could shelve #97 for 7.1, too?

@awelzel
Copy link
Contributor Author

awelzel commented Jun 25, 2024

@ckreibich - any thoughts here? We could shelve #97 for 7.1, too?

I am closing this. Nothing for 7.0 at this point.

@awelzel awelzel closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsmmr rsmmr Awaiting requested review from rsmmr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@awelzel @rsmmr